Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a noMatchSnapshots option. #917

Closed
wants to merge 4 commits into from
Closed

Add a noMatchSnapshots option. #917

wants to merge 4 commits into from

Conversation

tmeasday
Copy link
Member

See (storybook-eol/storyshots#93).

Issue:

The idea is that early on in a project, when things are changing a lot, snapshot tests are not particularly useful, but it's still helpful to "smoke test" all stories to ensure they run without error.

What I did

I added a noMatchSnapshots option to just setup a test that runs each story, without checking snapshots.

How to test

  1. Create a project, install storyshots but pass initStoryshots({ noMatchSnapshots: true }).
  2. Add a story, run jest. Should pass.
  3. Change the component's output, run jest again. Should still pass
  4. Make the component error in the render function, run jest again. Should fail.

Notes:

Perhaps it would make sense to just allow passing a custom test function, to cover off cases like this and storybook-eol/storyshots#76? Then storyshots is more of a generic tool to integrate storybook with Jest.

initStoryshots({
  test(renderedStory) {
     // In my case, just render without looking at the result
     renderer.create(renderedStory).toJSON()

     // For #76, shallow render + snapshot
     const tree = shallowToJson(shallow(renderedStory))
     expect(tree).toMatchSnapshot()
  }
});

@tmeasday tmeasday requested review from ndelangen and shilman and removed request for ndelangen April 19, 2017 12:40
shilman
shilman requested changes Apr 24, 2017
View reviewed changes
packages/storyshots/README.md
@@ -97,3 +97,7 @@ initStoryshots({
```

Here is an example of [a regex](https://regex101.com/r/vkBaAt/2) which does not pass if `"Relay"` is in the name: `/^((?!(r|R)elay).)*$/`.

### `noMatchSnapshots`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmeasday What do you think about creating an option compareSnapshots that defaults to true? I don't like the double negative of noMatchSnapshots. Or just call the option you added skipComparison?

@shilman
Copy link
Member

shilman commented Apr 24, 2017

@tmeasday FWIW, I prefer your genericized solution to the current proposal.

@shilman
Copy link
Member

shilman commented Apr 24, 2017 •
edited
Loading

I'm having problems testing out the change. It's probably unrelated to your change. What I did:

cd storybook
npm install
cd packages/storyshots
npm link .
cd ../../examples/cra-storybook
npm link storyshots
emacs src/storyshots.tests.js
npm run test

With the following contents for storyshots.tests.js:

import initStoryshots from 'storyshots'
initStoryshots({ noMatchSnapshots: true })

I get the error:

  ● Test suite failed to run

    Cannot find module 'react-test-renderer' from 'index.js'
      
      at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:151:17)
      at Object.<anonymous> (../../../../../.nvm/versions/node/v6.2.1/lib/node_modules/storyshots/dist/index.js:13:26)
      at Object.<anonymous> (src/storyshots.test.js:1:173)

I tried npm i -D react-test-renderer to add the peer dependency, but still get the same error.

I suspect this is related to Lerna. @tmeasday @ndelangen @aaronmcadam. Any help would be much appreciated.

ndelangen
ndelangen reviewed Apr 24, 2017
View reviewed changes
Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd name this autoUpdate and have it default value be false.

But I'm not really feeling much for this feature myself, since updating snapshots is real easy already anyway.

@tmeasday
Copy link
Member Author

I'm having problems testing out the change

Apologies, I didn't test the changes on the monorepo. My bad. Let me have a go at it and I'll ping you when I figure it out.

@tmeasday
Copy link
Member Author

I'd name this autoUpdate and have it default value be false.

But I'm not really feeling much for this feature myself, since updating snapshots is real easy already anyway.

@ndelangen -- the idea isn't to update the snapshots, it's to "smoke test" your stories but not to snapshot them.

When you are early in development, pretty much every commit changes your snapshots, and so (in my experience) you tend to disable snapshot tests as they are too noisy. However it's pretty useful to know if a commit has broken a component (in the sense that it fails to render for some story). That's the idea of this change.

@shilman -- I tend to agree that we should do the more generic thing.

@tmeasday
Make storybook et al peer deps.
b78f337 
Storyshots wants to plugin to the version used by the app, not have its own version. So its a clear peer dep.

Not sure it should depend on *storybook* at all given it is optional whether the user uses it or `react-native-storybook`
@tmeasday
Use storyshots in the CRA test app
10d74a3
@tmeasday
Add a noMatchSnapshots option.
c1c0489
@tmeasday tmeasday force-pushed the storyshots-no-match-snapshots branch from 7cccef7 to c1c0489 Compare April 28, 2017 10:42
@tmeasday
Copy link
Member Author

I rebased this onto #971

Should be easy to test now @shilman.

Haven't changed the API yet though, LMK if it works for you and I'll look at that.

This was referenced May 1, 2017
Merged
Closed
This was referenced May 15, 2017
Closed
Closed
@tmeasday
Copy link
Member Author

Closing this in favour of #1034, simple PR to follow

@tmeasday tmeasday closed this May 15, 2017
@shilman shilman added the misc label May 27, 2017
@lili2311 lili2311 mentioned this pull request Sep 25, 2023
Open
This was referenced Nov 25, 2023
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndelangen ndelangen ndelangen left review comments

@shilman shilman shilman requested changes

Assignees

@tmeasday tmeasday

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tmeasday @shilman @ndelangen